chore: update to electron 9-x-y #102011

deepak1556 · 2020-07-09T17:58:36Z

Fixes #100740
Fixes #98400
Fixes #101069
Fixes #88873
Fixes #78867

Also fixes https://github.com/microsoft/vscode/issues?q=is%3Aopen+is%3Aissue+label%3Afixed-in-electron-8

TODO

Fix disabling crash reporter via argv.json Electron: initialise crash reporter before app.on("ready") #97123

src/main.js

build/azure-pipelines/product-build.yml

src/bootstrap-fork.js

src/vs/workbench/contrib/welcome/telemetryOptOut/browser/telemetryOptOut.ts

src/main.js

bpasero · 2020-07-10T06:59:59Z

One thing I forgot: we need to offer the user a transition path if the user has configured the crash reporter to be disabled in settings. E.g. we could check this setting on startup and update the argv.json automatically + maybe inform the user that a restart is needed.

bpasero · 2020-07-10T11:46:33Z

Converting to draft, please ping when ready for final reivew.

deepak1556 · 2020-07-10T11:48:06Z

@bpasero I have addressed the feedback, only work left is to accept the crash reporter extra parameters in the appcenter backend. PR in general is ready for review.

bpasero

Besides my comments in code, what is the plan to transition users to argv.json that today have the crash reporter disabled in settings?

src/vs/workbench/contrib/welcome/telemetryOptOut/browser/telemetryOptOut.ts

src/vs/workbench/services/extensions/electron-browser/localProcessExtensionHost.ts

src/vs/workbench/electron-browser/window.ts

src/bootstrap-fork.js

src/vs/workbench/electron-browser/window.ts

src/vs/workbench/services/extensions/electron-browser/localProcessExtensionHost.ts

src/main.js

deepak1556 · 2020-07-11T21:20:29Z

@bpasero I have refactored the crashreporter implementation yet again due to some regressions/complexities faced during testing.

First idea was to send the appcenter specific data uid, iid, sid as extra parameters with crashReporter.addExtraParameter. The downside of this api is that the crash report will contain these extra parameters only in the process in which this api is called, that means we have to make sure to call the api in main, all the different renderers and node child processes, which increases the complexity of the implementation.
Instead I have taken the initial approach to encode all these data in the crash reporter startup which by default will carry these data for all processes. Only thing is that I have eliminated the use of sessionId from telemetryInfo because we are not using it in appcenter before and trying to add that info in the main process will only increase the complexity without any added benefits. And instead of reading from the storage service which uses sqlitedb underneath I have added a simpe one time unique crashReporterId that will be stored under the user data dir.
On linux, for child node processes the crash reporter has to be started explicitly, for which we share the crashReporterId from the previous step via command line that again eliminates doing any filesystem operations.
For disabling the appcenter crash reporter there are now 3 ways
- Using --crash-reporter-directory which will save the crash reports locally
- Setting "disable-crash-reporter": true in Configure runtime arguments
- Using --disable-crash-reporter from the command line

Have verified this to work for all 3 platforms and for all the processes we are interested in.

bpasero

Yeah I think we are getting to a nicer solution slowly. More feedback provided.

src/main.js

src/bootstrap-fork.js

src/main.js

bpasero

Found some quirks still, commented inline. Maybe it is easier we walk through the code together and you make some notes.

src/vs/workbench/services/extensions/electron-browser/localProcessExtensionHost.ts

src/main.js

src/vs/workbench/contrib/welcome/telemetryOptOut/browser/telemetryOptOut.ts

src/vs/workbench/electron-browser/window.ts

src/main.js

deepak1556 · 2020-07-13T23:20:34Z

src/vs/code/electron-main/app.ts

+		const argvString = argvContent.value.toString();
+		const argvJSON = JSON.parse(stripComments(argvString));
+		if (argvJSON['enable-crash-reporter'] === undefined) {
+			const enableCrashReporter = this.configurationService.getValue<boolean>('telemetry.enableCrashReporter');


this returns undefined in the main process. Is it too early to check the value ?

@deepak1556 configuration service in electron-main has no defaults because they get registered in renderer only. @sandy081 might have an issue for that, but for now we have to fill in the default value manually.

Yeah this is the limitation of using configuration service in non renderer process (main, shared). There could be an issue exists already but not sure.

bpasero

Only few comments left, getting close 👍

src/vs/workbench/electron-sandbox/desktop.contribution.ts

src/vs/platform/environment/node/argv.ts

src/main.js

deepak1556 · 2020-07-14T20:58:15Z

@bpasero should be good to go, PTAL.

bpasero

Nice 👍

deepak1556 · 2020-07-21T06:07:34Z

Internal builds with working devtools have been updated in vscode-electron-prebuilt, this PR is ready to be landed.

bpasero · 2020-07-21T06:22:30Z

@deepak1556 let me do the app.focus change we talked about and then merge this in?

deepak1556 · 2020-07-21T06:28:57Z

sounds good, feel free to merge the PR at your convenience. I am gonna sign-off for the day :)

bpasero · 2020-07-21T07:06:50Z

remote/.yarnrc

@@ -1,3 +1,3 @@
 disturl "http://nodejs.org/dist"
-target "12.4.0"
+target "12.14.1"


@alexdima fyi not sure why remote locked the dependency to 12.4.0 but it would now advance to the same as Electron 9

👍 Maybe there was no node release with the same version number as the one used in Electron?

* chore: bump electron@9.0.5 * remove exploration config * fix compile error * fix compile error * crashReporter has to be called only once before app ready * chore: bump electron@9.1.0 * enable LayoutNG * fix cron schedule * allow disabling appcenter crash reporting * set additional crash reporting parameters * start crashreporter for child process on linux * setup crash parameters only once * remove unused crashReporter.guid * address review feedback * reuse argv.json for storing crash reporter id * remove trailing commas * update localized name * update argv based on telemetry optout * update initial config based on setting * fix conditional errors * remove telemetry.enableCrashReporter * move default crash reporter config to electron-main * update comment for ext host crash reporting * set default value for configuration * some 💄 changes * address review feedback * do not use ES7 features in JS yet * add app.focus({ steal: true }) usage Co-authored-by: Benjamin Pasero <benjpas@microsoft.com>

This reverts commit 3d0d50c.

* chore: bump electron@9.0.5 * remove exploration config * fix compile error * fix compile error * crashReporter has to be called only once before app ready * chore: bump electron@9.1.0 * enable LayoutNG * fix cron schedule * allow disabling appcenter crash reporting * set additional crash reporting parameters * start crashreporter for child process on linux * setup crash parameters only once * remove unused crashReporter.guid * address review feedback * reuse argv.json for storing crash reporter id * remove trailing commas * update localized name * update argv based on telemetry optout * update initial config based on setting * fix conditional errors * remove telemetry.enableCrashReporter * move default crash reporter config to electron-main * update comment for ext host crash reporting * set default value for configuration * some 💄 changes * address review feedback * do not use ES7 features in JS yet * add app.focus({ steal: true }) usage Co-authored-by: Benjamin Pasero <benjpas@microsoft.com>

Co-authored-by: Benjamin Pasero <benjpas@microsoft.com>

deepak1556 requested a review from bpasero July 9, 2020 18:34

deepak1556 self-assigned this Jul 9, 2020

deepak1556 added this to the July 2020 milestone Jul 9, 2020

deepak1556 commented Jul 9, 2020

View reviewed changes

src/main.js Outdated Show resolved Hide resolved

bpasero requested changes Jul 10, 2020

View reviewed changes

bpasero self-requested a review July 10, 2020 11:46

bpasero marked this pull request as draft July 10, 2020 11:46

bpasero marked this pull request as ready for review July 10, 2020 11:53

bpasero requested changes Jul 11, 2020

View reviewed changes

gjsjohnmurray mentioned this pull request Jul 11, 2020

Hope to upgrade the chrome kernel version to electron 9 as soon as possible. #102216

Closed

deepak1556 marked this pull request as draft July 11, 2020 20:25

deepak1556 force-pushed the robo/update_electron_9 branch from 4973bd8 to 7d2a02f Compare July 11, 2020 22:33

deepak1556 marked this pull request as ready for review July 12, 2020 04:34

bpasero requested changes Jul 12, 2020

View reviewed changes

src/main.js Outdated Show resolved Hide resolved

src/bootstrap-fork.js Outdated Show resolved Hide resolved

src/main.js Outdated Show resolved Hide resolved

src/main.js Show resolved Hide resolved

deepak1556 marked this pull request as draft July 13, 2020 06:08

deepak1556 marked this pull request as ready for review July 13, 2020 06:36

bpasero requested changes Jul 13, 2020

View reviewed changes

deepak1556 commented Jul 13, 2020

View reviewed changes

bpasero requested changes Jul 14, 2020

View reviewed changes

src/vs/workbench/electron-sandbox/desktop.contribution.ts Outdated Show resolved Hide resolved

src/vs/platform/environment/node/argv.ts Show resolved Hide resolved

src/main.js Show resolved Hide resolved

sweettyler mentioned this pull request Jul 14, 2020

Crash on startup with remote X server #101069

Open

bpasero self-requested a review July 15, 2020 04:29

bpasero approved these changes Jul 15, 2020

View reviewed changes

deepak1556 and others added 3 commits July 20, 2020 22:50

chore: bump electron@9.0.5

f12a333

remove exploration config

2bda050

fix compile error

9f20666

deepak1556 and others added 12 commits July 20, 2020 22:58

remove trailing commas

5318e66

update localized name

0bbc904

update argv based on telemetry optout

b35b4a4

update initial config based on setting

f7dc3e8

fix conditional errors

a4a9c80

remove telemetry.enableCrashReporter

e9e9c6d

move default crash reporter config to electron-main

a12d098

update comment for ext host crash reporting

c6350ac

set default value for configuration

089291a

some 💄 changes

76c1c15

address review feedback

2978594

do not use ES7 features in JS yet

7bce18e

deepak1556 force-pushed the robo/update_electron_9 branch from bee1836 to 7bce18e Compare July 21, 2020 06:06

bpasero reviewed Jul 21, 2020

View reviewed changes

add app.focus({ steal: true }) usage

70b38e5

bpasero mentioned this pull request Jul 21, 2020

fix: window focus for second instance on macOS #98279

Closed

bpasero merged commit 3d0d50c into master Jul 21, 2020

bpasero deleted the robo/update_electron_9 branch July 21, 2020 07:39

deepak1556 mentioned this pull request Jul 21, 2020

Fix appcenter crash reporting on linux #103051

Closed

deepak1556 added a commit that referenced this pull request Jul 30, 2020

Revert "chore: update to electron 9-x-y (#102011)"

c77491f

This reverts commit 3d0d50c.

deepak1556 added a commit that referenced this pull request Jul 30, 2020

Revert "chore: update to electron 9-x-y (#102011)" (#103599)

227d533

This reverts commit 3d0d50c.

deepak1556 added a commit that referenced this pull request Aug 11, 2020

chore: update to electron 9-x-y (#102011)

e429633

Co-authored-by: Benjamin Pasero <benjpas@microsoft.com>

github-actions bot locked and limited conversation to collaborators Sep 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update to electron 9-x-y #102011

chore: update to electron 9-x-y #102011

deepak1556 commented Jul 9, 2020 •

edited

bpasero commented Jul 10, 2020

bpasero commented Jul 10, 2020

deepak1556 commented Jul 10, 2020 •

edited

bpasero left a comment

deepak1556 commented Jul 11, 2020 •

edited

bpasero left a comment

bpasero left a comment

deepak1556 Jul 13, 2020

bpasero Jul 14, 2020

sandy081 Jul 14, 2020

bpasero left a comment

deepak1556 commented Jul 14, 2020

bpasero left a comment

deepak1556 commented Jul 21, 2020

bpasero commented Jul 21, 2020

deepak1556 commented Jul 21, 2020

bpasero Jul 21, 2020

alexdima Jul 21, 2020

chore: update to electron 9-x-y #102011

chore: update to electron 9-x-y #102011

Conversation

deepak1556 commented Jul 9, 2020 • edited

bpasero commented Jul 10, 2020

bpasero commented Jul 10, 2020

deepak1556 commented Jul 10, 2020 • edited

bpasero left a comment

Choose a reason for hiding this comment

deepak1556 commented Jul 11, 2020 • edited

bpasero left a comment

Choose a reason for hiding this comment

bpasero left a comment

Choose a reason for hiding this comment

deepak1556 Jul 13, 2020

Choose a reason for hiding this comment

bpasero Jul 14, 2020

Choose a reason for hiding this comment

sandy081 Jul 14, 2020

Choose a reason for hiding this comment

bpasero left a comment

Choose a reason for hiding this comment

deepak1556 commented Jul 14, 2020

bpasero left a comment

Choose a reason for hiding this comment

deepak1556 commented Jul 21, 2020

bpasero commented Jul 21, 2020

deepak1556 commented Jul 21, 2020

bpasero Jul 21, 2020

Choose a reason for hiding this comment

alexdima Jul 21, 2020

Choose a reason for hiding this comment

deepak1556 commented Jul 9, 2020 •

edited

deepak1556 commented Jul 10, 2020 •

edited

deepak1556 commented Jul 11, 2020 •

edited